[Access] Add implementation BlockProvider #6636
Conversation
… and block provider
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6636 +/- ##
==========================================
- Coverage 41.05% 41.05% -0.01%
==========================================
Files 2071 2076 +5
Lines 183431 183653 +222
==========================================
+ Hits 75303 75393 +90
- Misses 101823 101962 +139
+ Partials 6305 6298 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
engine/access/rest/websockets/data_providers/blocks_provider.go
Outdated
Show resolved
Hide resolved
…d ctx from HandleSubscription
engine/access/rest/websockets/data_providers/blocks_provider.go
Outdated
Show resolved
Hide resolved
engine/access/rest/websockets/data_providers/blocks_provider.go
Outdated
Show resolved
Hide resolved
engine/access/rest/websockets/data_providers/blocks_provider.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Andrii Slisarchuk <Guitarheroua@users.noreply.github.com>
|
Please, wrap LGTM |
…:The-K-R-O-K/flow-go into UlyanaAndrukhiv/6585-block-data-provider
engine/access/rest/websockets/data_providers/blocks_provider.go
Outdated
Show resolved
Hide resolved
…:The-K-R-O-K/flow-go into UlyanaAndrukhiv/6585-block-data-provider
There was a problem hiding this comment.
mostly smaller comments. I think you should expand on the data provider tests to include some happy path testing. I also think there's currently a gap that covers more end-to-end functional testing. That can come in a separate PR, but wondering if we will need more control than is possible using integration tests alone. what do you think?
engine/access/rest/websockets/data_providers/block_headers_provider.go
Outdated
Show resolved
Hide resolved
| type BlockMessageResponse struct { | ||
| // The sealed or finalized blocks according to the block status | ||
| // in the request. | ||
| Block *flow.Block `json:"block"` |
There was a problem hiding this comment.
Are these response objects finalized?
Should this be a dedicated DTO similar to what exists when using REST (see here)? Casing and naming on REST endpoints is different than what these models will serialize to if directly marshalled into JSON.
Consistency aside, I'm also not sure that these models will serialize "properly" to JSON out-of-box. On the REST endpoints, large numbers are serialized to JSON strings instead of numbers which prevents integer overflow on JS clients. Without a custom JSON parser, JS clients have a maximum safe precision of 53 bits.
There was a problem hiding this comment.
Hi Jordan! Yes, this is exactly what we missed. Thank you for pointing out this issue. As I see it, the solution could involve reusing the same converters and models as those for the REST API, or creating new converters similar to the ones we currently have for the REST API. We will discuss this and I come back to you with concrete answer.
There was a problem hiding this comment.
+1. the WS endpoint should return data in the same format as the REST endpoints.
There was a problem hiding this comment.
We have created a new issue to address this #6775
engine/access/rest/websockets/data_providers/blocks_provider_test.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
Closes: #6584 , #6585
Context
In this PR implemented :
DataProviderFactorywhich is responsible for creating data providers based on the requested topic.DataProviderwhich is the interface abstracts of the actual data provider used by theWebSocketCollector.BaseDataProviderandBaseDataProviderImpl- the interface and the concrete implementation which holds common objects for the provider.BlocksDataProviderwhich is responsible for providing blocks.BlockHeadersDataProviderwhich is responsible for providing block headers.BlockDigestsDataProviderwhich is responsible for providing block digests.Added unit tests to cover this functionality.